-
Notifications
You must be signed in to change notification settings - Fork 392
UIE-9379: Implementing Add Pool functionality for PgBouncer #13276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
UIE-9379: Implementing Add Pool functionality for PgBouncer #13276
Conversation
3595b01 to
eceb323
Compare
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
| border: `1px solid ${theme.tokens.component.Select.Error.Border}`, | ||
| } | ||
| : undefined; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drawer is currently using the CDS Select and Linode MUI TextField since the numeric spinner isn't available in the cds-web-components yet. This is to give the cds Select component the same error border color that gets used with the Linode components in their error state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we can also provide an error class to select, so I'm going to look into whether that gives the same border color. If so, we may be able to remove this and leverage that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing it out, it looks like providing className="error" to the Select component doesn't provide the error class to the component and, as a result, it won't display the error styles for that class from the component. I also tried wrapping it in a div with the error class name, but that didn't work either.
Is there a specific way this class needs to be applied to Select? Otherwise we may need to keep this as a workaround for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure since it's a CDS Select and it's different from how our UI Select component works. Maybe the CDS team would know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smans-akamai, your approach also seems fine.
I wasn't able to reproduce an error state for these selects since there are no empty options, but I was playing around with cds-web-components and also just adding a new select there:
you can add your className but you also need to use styled :
const StyledSelect = styled(Select)`
&.your-class {
...
}
`;
<StyledSelect
{...field}
className="your-class"
data-testid="database-name-select"
...
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaleksee-akamai Thanks, I'll look into this other approach. In the meantime, you can reproduce the error state in this pull request with the following steps:
- Uncomment the mock error response for the connection-pools POST request
- Go to the
Networkingtab of anactivePostgreSQL Database Clusterhttp://localhost:3000/databases/postgresql/1/networking - Select the
Add Poolbutton in theManage PgBouncer Connection Poolssection to open theAdd a New Connection Pooldrawer - Make a valid selection in the fields and select
Add Pool - Select submit and see that the POST returns an error response
- Verify that the backend errors are displayed below their respective fields.
I've included the steps in the description.
| // }, | ||
| // { status: 400 } | ||
| // ); | ||
| return HttpResponse.json(connectionPool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can uncomment the lines above to mock the error response for the fields to see the inline errors.
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseAddConnectionPoolDrawer.tsx
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
| className={classes.actionBtn} | ||
| disabled={true} | ||
| onClick={() => null} | ||
| disabled={database.status !== 'active'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add a tooltip to explain why the button is disabled. Something like:
| disabled={database.status !== 'active'} | |
| disabled={database.status !== 'active'} | |
| tooltipText={ | |
| database.status !== 'active' ? 'Cluster must be active' : '' | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! I've just made this change and included the following text:
To add a new connection pool, the database cluster must be active.
I used similar language to what we have for DatabaseManageNetworking but I can double check the language with tech writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm what about The database cluster must be active to add a new connection pool? Swap the wording around so that the reason is first 🤔
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
...src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPoolAddDrawer.tsx
Outdated
Show resolved
Hide resolved
| className={classes.actionBtn} | ||
| disabled={true} | ||
| onClick={() => null} | ||
| disabled={database.status !== 'active'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm what about The database cluster must be active to add a new connection pool? Swap the wording around so that the reason is first 🤔
| onClick={() => setIsAddPoolDrawerOpen(true)} | ||
| TooltipProps={{ placement: 'top' }} | ||
| tooltipText={ | ||
| database.status !== 'active' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a disabled variable so that the state is sync'd
dwiley-akamai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verification steps ✅
| /** Shared ErrorText component that displays formatted error messages below form field components that don't have the errorText property */ | ||
| const SharedErrorText = (errorMessage: string | undefined) => | ||
| errorMessage && ( | ||
| <FormHelperText | ||
| error | ||
| role="alert" | ||
| sx={{ marginTop: theme.spacingFunction(4) }} | ||
| > | ||
| {errorMessage} | ||
| </FormHelperText> | ||
| ); | ||
|
|
||
| /** Utility function to generate error styles for form field components that can't set them by default */ | ||
| const makeErrorStyles = (errorMessage: string | undefined) => { | ||
| return errorMessage | ||
| ? { | ||
| border: `1px solid ${theme.tokens.component.Select.Error.Border}`, | ||
| } | ||
| : undefined; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could consider moving these to the databases utilities file, especially if they may be used elsewhere for the feature
Cloud Manager UI test results🔺 1 failing test on test run #11 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts" |
|||||||||||||||||
| return `${poolMode.charAt(0).toUpperCase()}${poolMode.slice(1)}`; | ||
| }} | ||
| /> | ||
| {SharedErrorText(fieldState.error?.message)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems ok but just curious - why you don't use rules there?
| } | ||
| }; | ||
|
|
||
| const { mode, database, username } = watch(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use useWatch over watch for performance

Description 📝
This pull request is to implement the Add Pool functionality for PgBouncer. This will allow users to create new connection pools via a new drawer. Those connection pools will then be added to the connection pools table in the Networking tab.
Changes 🔄
List any change(s) relevant to the reviewer.
Add Poolbutton in the Database DetailsNetworkingtabManage PgBouncer Connection Poolssection to open an add connection pool drawer.Scope 🚢
Upon production release, changes in this PR will be visible to:
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Add Poolbutton gets enabled for postgresql database clusters, modify the mock database response by setting thedatabasestatus property in makeMockDatabase toactive.Verification steps
(How to verify changes)
General
Networkingtab of anactivePostgreSQL Database Clusterhttp://localhost:3000/databases/postgresql/1/networkingAdd Poolbutton in theManage PgBouncer Connection Poolssection to open theAdd a New Connection PooldrawerAdd PoolBackend Errors Displaying Below Fields
Networkingtab of anactivePostgreSQL Database Clusterhttp://localhost:3000/databases/postgresql/1/networkingAdd Poolbutton in theManage PgBouncer Connection Poolssection to open theAdd a New Connection PooldrawerAdd PoolAuthor Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅